-
Notifications
You must be signed in to change notification settings - Fork 16
Upgrade prettier past 3.6.0 #407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5cc7e52 to
50addc1
Compare
50addc1 to
8fdb8fb
Compare
|
Turns out tests break down to 3.6.0 |
|
These are the changes that broke everything Flipping these to true fixes everything. |
|
Can live with the ranges not being there, easy enough to grab them from start and end. Can't live with comments not being included. We (for reasons I don't understand yet) have all our template stuff come out as |
|
Down to 24 failures. Majority of them because |
|
apparently only 7 failures in CI 🤔 edit: I was running 3.6.0 locally, it seems 3.7.1 (or a version in between) fixed some things. |
|
the last 7 failures are down to a change in prettier I think, not sure when a template is passed as an arg to a function eg render(
<template>
Hello
</template>
);Prettier now collapses the whitespace around the arg resulting in render(<template>
Hello
</template>)I'm not sure if this is an ok change we can accept or not? |
|
One of those failures is a random semi-colon being inserted 😢 - Expected
+ Received
@@ -5,9 +5,9 @@
Args: {}
Yields: []
}
// prettier-ignore
- <template>
+ ;<template>
what
</template> as TemplateOnlyComponent<Signature>
"why? |
|
I've opened an issue with prettier, if they can revert the change this PR becomes much simpler in that it just needs to fix the missing range property. Otherwise I'll have to keep at it and figure out these last couple of issues. By changing the comments to an object we get a different AST, which is causing the last issues because they are not identifying as an |
src/parse/index.ts
Outdated
| node.range = [node.start, node.end]; | ||
| } | ||
| assert('expected range', node.range); | ||
| const [start, end] = node.range; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use typescript.{locStart,locEnd}() which works for all prettier js parsers, and won't break.
|
Superceded by #408 |
Upgrading broke many tests